[exporter/loadbalancing] Adding AWS Cloud Map for Load Balancer Exporter discovery #27588

andretong · 2023-10-10T12:55:15Z

Description:
Implementation of 27241

Adding AWS Service discovery system (CloudMap) support to resolve the Collector's Backend.
This would allow users to use this exporter when there is the case of using ECS over EKS in an AWS infrastructure.

Link to tracking Issue:

Testing:

Setting the following configuration with the proper values, we can start the Load Balancer which will connect to the AWS CloudMap service and start sending traces to the available collectors.

exporters:
  loadbalancing:
    protocol:
      otlp:
        timeout: 3s
    resolver:
      awsCloudMap:
        namespace: <aws-namespace>
        serviceName: <aws-otel-col-service-name>
        interval: <interval-in-seconds>

Documentation:

linux-foundation-easycla · 2023-10-10T12:55:20Z

The committers listed above are authorized under a signed CLA.

✅ login: andretong / name: Andre Tong (cd62b25, 5648637, dd3b909, 8c4ee18, 9c20708, b19f4ca, 2579222, a11df8d, 0ac5581, 9c6a77d)

andretong · 2023-10-10T15:47:21Z

exporter/loadbalancingexporter/README.md

@@ -53,7 +53,7 @@ This also supports service name based exporting for traces. If you have two or m
 Refer to [config.yaml](./testdata/config.yaml) for detailed examples on using the processor.

 * The `otlp` property configures the template used for building the OTLP exporter. Refer to the OTLP Exporter documentation for information on which options are available. Note that the `endpoint` property should not be set and will be overridden by this exporter with the backend endpoint.
-* The `resolver` accepts a `static` node, a `dns` or a `k8s` service. If all three are specified, `k8s` takes precedence.
+* The `resolver` accepts a `static` node, a `dns`, a `k8s` service or `awsCloudMap`. If all four are specified, `k8s` takes precedence.


TODO:
Define what to do in this case since currently when static and dns are defined, then throws a errMultipleResolversProvided

opentelemetry-collector-contrib/exporter/loadbalancingexporter/loadbalancer.go

Lines 55 to 57 in 0efc53a

if oCfg.Resolver.DNS != nil && oCfg.Resolver.Static != nil {

return nil, errMultipleResolversProvided

}

I think it makes sense to return an error, and users should have this expectation as well. Can you please change this line in the doc to reflect what the code does?

jpkrohling

This looks great! Thank you for your PR. @Aneurysm9, @bryan-aguilar, would someone from AWS be available to review this PR?

jpkrohling · 2023-10-24T09:31:07Z

exporter/loadbalancingexporter/README.md

@@ -53,7 +53,7 @@ This also supports service name based exporting for traces. If you have two or m
 Refer to [config.yaml](./testdata/config.yaml) for detailed examples on using the processor.

 * The `otlp` property configures the template used for building the OTLP exporter. Refer to the OTLP Exporter documentation for information on which options are available. Note that the `endpoint` property should not be set and will be overridden by this exporter with the backend endpoint.
-* The `resolver` accepts a `static` node, a `dns` or a `k8s` service. If all three are specified, `k8s` takes precedence.
+* The `resolver` accepts a `static` node, a `dns`, a `k8s` service or `awsCloudMap`. If all four are specified, `k8s` takes precedence.


I think it makes sense to return an error, and users should have this expectation as well. Can you please change this line in the doc to reflect what the code does?

jpkrohling · 2023-10-24T09:32:58Z

exporter/loadbalancingexporter/README.md

+  * `namespace` The CloudMap namespace where the service is register, e.g. `cloudmap`. If no `namespace` is specified, this will fail to start the Load Balancer exporter.
+  * `serviceName` The name of the service that you specified when you registered the instance, e.g. `otelcollectors`.  If no `serviceName` is specified, this will fail to start the Load Balancer exporter.
+  * `interval` resolver interval in go-Duration format, e.g. `5s`, `1d`, `30m`. If not specified, `30s` will be used.
+  * `timeout` resolver timeout in go-Duration format, e.g. `5s`, `1d`, `30m`. If not specified, `1s` will be used.


Is 1s a reasonable value here? If the default interval is 30s, the timeout can certainly be at around 5s, no?

Yes, makes all the sense, I've updated this property and documentation

jpkrohling · 2023-10-24T09:33:40Z

exporter/loadbalancingexporter/README.md

@@ -63,6 +63,12 @@ Refer to [config.yaml](./testdata/config.yaml) for detailed examples on using th
 * The `k8s` node accepts the following optional properties:
  * `service` Kubernetes service to resolve, e.g. `lb-svc.lb-ns`. If no namespace is specified, an attempt will be made to infer the namespace for this collector, and if this fails it will fall back to the `default` namespace.
  * `ports` port to be used for exporting the traces to the addresses resolved from `service`. If `ports` is not specified, the default port 4317 is used. When multiple ports are specified, two backends are added to the load balancer as if they were at different pods.
+* The `awsCloudMap` node accepts the following properties:


I know nothing about AWS Cloud Map, is there anything users have to be aware before configuring it?

The only relevant topic I can think of is that usage of the SDK hits the API every polling interval, so users have to take into account the AWS API Rate limits in their configuration.

Can this comment be added to the readme? Or is it so obvious that it doesn't have to?

jpkrohling · 2023-10-24T09:34:40Z

exporter/loadbalancingexporter/README.md

+  * `serviceName` The name of the service that you specified when you registered the instance, e.g. `otelcollectors`.  If no `serviceName` is specified, this will fail to start the Load Balancer exporter.
+  * `interval` resolver interval in go-Duration format, e.g. `5s`, `1d`, `30m`. If not specified, `30s` will be used.
+  * `timeout` resolver timeout in go-Duration format, e.g. `5s`, `1d`, `30m`. If not specified, `1s` will be used.
+  * `port` port to be used for exporting the traces to the addresses resolved from `service`. By default, the port is set in Cloud Map, but can be be overridden with a static value in this config


There seems to be a healthStatus property as well. Should it be documented?

For this property, in this initial iteration, I'd rather to leave this config as default since we filter only for Healthy instances

Reference: https://docs.aws.amazon.com/cloud-map/latest/api/API_GetInstancesHealthStatus.html

I might be missing something, but the value the user is providing is indeed being used, and therefore, deserves to be documented.

jpkrohling · 2023-10-24T09:38:40Z

exporter/loadbalancingexporter/loadbalancer.go

@@ -52,7 +52,20 @@ type loadBalancerImp struct {
 func newLoadBalancer(params exporter.CreateSettings, cfg component.Config, factory componentFactory) (*loadBalancerImp, error) {
 	oCfg := cfg.(*Config)

-	if oCfg.Resolver.DNS != nil && oCfg.Resolver.Static != nil {
+	var count = 0


This works, and I like that it provides a short-circuit feature to the function. But I propose two alternatives:

externalize this into a func checkMultipleResolversProvided(Config) error, and call this function here. It should make your code easier to test and leave the existing function more readable.

alternatively, you can create a boolean configured, and set it to true whenever a resolver is not nil. On the next resolver, you check if configured is true and return the errMultipleResolversProvided error. You won't have the fail-fast behavior, though.

exporter/loadbalancingexporter/resolver_aws_cloudmap.go

jpkrohling · 2023-10-24T09:46:50Z

exporter/loadbalancingexporter/resolver_aws_cloudmap.go

+
+func (r *cloudMapResolver) start(ctx context.Context) error {
+	if _, err := r.resolve(ctx); err != nil {
+		r.logger.Warn("Failed initial resolve", zap.Error(err))


Suggested change

r.logger.Warn("Failed initial resolve", zap.Error(err))

r.logger.Warn("failed initial resolve", zap.Error(err))

jpkrohling · 2023-10-24T09:51:19Z

exporter/loadbalancingexporter/resolver_aws_cloudmap.go

+
+	_ = stats.RecordWithTags(ctx, awsResolverSuccessTrueMutators, mNumResolutions.M(1))
+
+	r.logger.Debug("DiscoverInstance",


I think this deserves a better message.

jpkrohling · 2023-10-24T09:51:54Z

exporter/loadbalancingexporter/resolver_aws_cloudmap.go

+		} else {
+			endpoint = fmt.Sprintf("%s:%d", ipAddr, *r.port)
+		}
+		r.logger.Debug("Resolved instance",


Suggested change

r.logger.Debug("Resolved instance",

r.logger.Debug("resolved instance",

0x006EA1E5 · 2023-11-15T09:35:26Z

Hey, this looks great. Can we expect this to be available in the next release?

jpkrohling · 2023-11-23T14:07:40Z

exporter/loadbalancingexporter/resolver_aws_cloudmap.go

+
+func createDiscoveryFunction(client *servicediscovery.Client) func(params *servicediscovery.DiscoverInstancesInput) (*servicediscovery.DiscoverInstancesOutput, error) {
+	return func(params *servicediscovery.DiscoverInstancesInput) (*servicediscovery.DiscoverInstancesOutput, error) {
+		return client.DiscoverInstances(context.TODO(), params)


It was brought to my attention that this returns "only" the first 100 hosts, and that the results to this API are paginated. Can you handle this in this PR? If you prefer to handle that in a future PR, please open an issue and leave a reference to it here.

For this specific case, I'll rather add this pagination as a new iteration.
Will raise the issue as requested.

Thanks for the review.

Here is the issue raised described to handle this limitation. #29771

Is there a way to know whether there are more than 100 hosts available? If so, perhaps it could warn a message in the logs?

jpkrohling · 2023-11-23T14:09:57Z

exporter/loadbalancingexporter/loadbalancer.go

@@ -198,3 +208,27 @@ func (lb *loadBalancerImp) Exporter(endpoint string) (component.Component, error

 	return exp, nil
 }
+
+func checkMultipleResolversProvided(cfg component.Config) (bool, error) {


I'm not entirely comfortable with this change in this PR. I think it has to be on its own PR.

Sure, I'll raise a new PR once this is merge with the increment.

jpkrohling · 2023-12-12T10:29:30Z

@andretong, once this is ready for another review, let me know but note that I'm unable to do so this year. I'm back on Jan 15, please ping me again on Jan 18 if I haven't reviewed this here by then.

andretong · 2023-12-12T14:05:30Z

@andretong, once this is ready for another review, let me know but note that I'm unable to do so this year. I'm back on Jan 15, please ping me again on Jan 18 if I haven't reviewed this here by then.

@jpkrohling thanks for the notice.
All the suggestions were applied and waiting for a review, looking forward to get back with you regarding this feature.

github-actions · 2023-12-27T05:19:19Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

andretong · 2024-03-12T09:32:19Z

Another failure :-/

Error: exporter/awskinesisexporter/exporter.go:10:2: missing go.sum entry for module providing package github.com/aws/aws-sdk-go-v2/aws (imported by github.com/open-telemetry/opentelemetry-collector-contrib/exporter/awskinesisexporter); to add:
	go get github.com/open-telemetry/opentelemetry-collector-contrib/exporter/[email protected]

@jpkrohling I think I've fixed the errors, can you run the pipeline please?

I've check locally and the distribution is built when running make otelcontribcol

andretong · 2024-03-13T09:22:16Z

@jpkrohling I've uploaded the missing artifact in the last commit.

andretong · 2024-03-13T13:17:50Z

@jpkrohling seems is OK but a need rebase was needed :(

andretong · 2024-03-13T14:48:02Z

@jpkrohling all checks have pass :D

mx-psi · 2024-03-13T22:02:22Z

Merging based on #27588 (comment) cc @jpkrohling

github-actions bot added cmd/otelcontribcol otelcontribcol command exporter/loadbalancing labels Oct 10, 2023

github-actions bot requested a review from jpkrohling October 10, 2023 12:55

andretong force-pushed the feature/loadbalancingexporter_aws_support branch from 436e611 to 56a9501 Compare October 10, 2023 12:59

andretong marked this pull request as ready for review October 10, 2023 13:03

andretong requested a review from a team October 10, 2023 13:03

github-actions bot assigned andrzej-stencel Oct 10, 2023

andretong force-pushed the feature/loadbalancingexporter_aws_support branch 2 times, most recently from 10bedf5 to 00aa631 Compare October 10, 2023 15:29

andretong commented Oct 10, 2023

View reviewed changes

andretong force-pushed the feature/loadbalancingexporter_aws_support branch 2 times, most recently from 9575682 to caa3636 Compare October 14, 2023 17:54

jpkrohling approved these changes Oct 24, 2023

View reviewed changes

andretong force-pushed the feature/loadbalancingexporter_aws_support branch 3 times, most recently from 62747e7 to e7f13f2 Compare November 7, 2023 09:15

andretong requested review from mx-psi and dmitryax as code owners November 7, 2023 09:15

github-actions bot added the cmd/configschema configschema command label Nov 7, 2023

andretong force-pushed the feature/loadbalancingexporter_aws_support branch from e7f13f2 to 573268f Compare November 7, 2023 09:17

jpkrohling reviewed Nov 23, 2023

View reviewed changes

andretong force-pushed the feature/loadbalancingexporter_aws_support branch 2 times, most recently from a065ed4 to 64f1eaf Compare November 30, 2023 14:48

andretong force-pushed the feature/loadbalancingexporter_aws_support branch from 64f1eaf to 2cf2d4b Compare December 12, 2023 13:38

andretong mentioned this pull request Dec 12, 2023

AWS Cloud Map usage at Load Balancing has a max limit up to 100 instances #29771

Open

andretong force-pushed the feature/loadbalancingexporter_aws_support branch 3 times, most recently from bee7b81 to 840f2ef Compare March 12, 2024 09:08

andretong force-pushed the feature/loadbalancingexporter_aws_support branch 4 times, most recently from 51cb8ef to c9776bf Compare March 13, 2024 07:34

andretong force-pushed the feature/loadbalancingexporter_aws_support branch 2 times, most recently from a2fa3b8 to 70eddfa Compare March 13, 2024 13:17

andretong added 10 commits March 13, 2024 19:24

Adding AWS Cloud Map for Load Balancer Exporter discovery

dd3b909

rollback upgrade

cd62b25

update go.mod loadbalancer

2579222

rollback changes in otelcontrib folder

5648637

update go.sum loadbalancer

9c20708

rollback on aws-sdk-v2 to 1.25.2

b19f4ca

rollback updates at configschema

8c4ee18

go tidy and updates

a11df8d

update aws config sdk

9c6a77d

update with genotelcontribcol

0ac5581

andretong force-pushed the feature/loadbalancingexporter_aws_support branch from 70eddfa to 0ac5581 Compare March 13, 2024 18:24

mx-psi merged commit e7e779f into open-telemetry:main Mar 13, 2024
142 checks passed

github-actions bot added this to the next release milestone Mar 13, 2024

mx-psi mentioned this pull request Mar 27, 2024

[exporter/loadbalancing] AWS Cloud Map to resolve collectors backends #27241

Closed

elikatsis mentioned this pull request Apr 10, 2024

[exporter/loadbalancing] The awsCloudMap tag doesn't satisfy the config tag regular expression #32296

Closed

dmitryax mentioned this pull request Apr 11, 2024

[exporter/loadbalancing] Change config fields to use snake_case #32331

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[exporter/loadbalancing] Adding AWS Cloud Map for Load Balancer Exporter discovery #27588

[exporter/loadbalancing] Adding AWS Cloud Map for Load Balancer Exporter discovery #27588

andretong commented Oct 10, 2023

linux-foundation-easycla bot commented Oct 10, 2023 •

edited

Loading

andretong Oct 10, 2023

jpkrohling Oct 24, 2023

jpkrohling left a comment •

edited

Loading

jpkrohling Oct 24, 2023

jpkrohling Oct 24, 2023

andretong Nov 6, 2023

jpkrohling Oct 24, 2023

andretong Nov 6, 2023

jpkrohling Jan 29, 2024

jpkrohling Oct 24, 2023

andretong Nov 6, 2023

jpkrohling Jan 29, 2024

jpkrohling Oct 24, 2023

jpkrohling Oct 24, 2023

jpkrohling Oct 24, 2023

jpkrohling Oct 24, 2023

0x006EA1E5 commented Nov 15, 2023

jpkrohling Nov 23, 2023

andretong Nov 30, 2023

andretong Dec 12, 2023

jpkrohling Jan 29, 2024

jpkrohling Nov 23, 2023

andretong Nov 30, 2023

jpkrohling commented Dec 12, 2023

andretong commented Dec 12, 2023

github-actions bot commented Dec 27, 2023

andretong commented Mar 12, 2024

andretong commented Mar 13, 2024

andretong commented Mar 13, 2024

andretong commented Mar 13, 2024

mx-psi commented Mar 13, 2024

	if oCfg.Resolver.DNS != nil && oCfg.Resolver.Static != nil {
	return nil, errMultipleResolversProvided
	}

	r.logger.Warn("Failed initial resolve", zap.Error(err))
	r.logger.Warn("failed initial resolve", zap.Error(err))


		_ = stats.RecordWithTags(ctx, awsResolverSuccessTrueMutators, mNumResolutions.M(1))

		r.logger.Debug("DiscoverInstance",

	r.logger.Debug("Resolved instance",
	r.logger.Debug("resolved instance",

[exporter/loadbalancing] Adding AWS Cloud Map for Load Balancer Exporter discovery #27588

[exporter/loadbalancing] Adding AWS Cloud Map for Load Balancer Exporter discovery #27588

Conversation

andretong commented Oct 10, 2023

linux-foundation-easycla bot commented Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x006EA1E5 commented Nov 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling commented Dec 12, 2023

andretong commented Dec 12, 2023

github-actions bot commented Dec 27, 2023

andretong commented Mar 12, 2024

andretong commented Mar 13, 2024

andretong commented Mar 13, 2024

andretong commented Mar 13, 2024

mx-psi commented Mar 13, 2024

linux-foundation-easycla bot commented Oct 10, 2023 •

edited

Loading

jpkrohling left a comment •

edited

Loading